Fix that RemoveUser doesn't work when create Public Client with broker #27448
Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses an issue with RemoveUser when creating a public client with broker support, by propagating an authority parameter through various authentication calls. Key changes include:
- Introducing new RemoveUser overloads that accept an authority string in the authentication factories and mocks.
- Propagating the authority parameter to public client creation and token cache removal methods.
- Updating related context and cache clearing logic to use the provided authority.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Accounts/Authentication/Factories/AuthenticationFactory.cs | Added RemoveUser overload and updated RemoveFromTokenCache to pass authority. |
| src/Accounts/Accounts/Context/GetAzureRMContext.cs | Updated context refresh logic by setting the environment property before refreshing contexts. |
| src/Accounts/Accounts/Account/DisconnectAzureRmAccount.cs | Changed RemoveUser call to pass the authority from the current context. |
| src/Accounts/Accounts/Context/RemoveAzureRmContext.cs | Updated TryRemoveAccount call to include the authority parameter. |
| tools/TestFx/Mocks/MockTokenAuthenticationFactory.cs tools/TestFx/Mocks/MockCertificateAuthenticationFactory.cs |
Added new RemoveUser overloads that throw NotImplementedException. |
| src/Accounts/Accounts/Context/ClearAzureRmContext.cs | Updated clearing of token cache to pass the authority from the default context. |
| src/Accounts/Authentication/ResourceManager/AzureRmProfile.cs | Updated authority handling in RefreshContextsFromCache and maintained a fixme comment regarding Connect-AzAccount. |
| src/Accounts/Authentication/Authentication/TokenCache/InMemoryTokenCacheProvider.cs SharedTokenCacheProvider.cs PowerShellTokenCacheProvider.cs |
Changed ClearCache and other token cache related methods to accept an authority parameter. |
Comments suppressed due to low confidence (2)
src/Accounts/Accounts/Context/GetAzureRMContext.cs:87
- Add null checks for 'DefaultProfile.DefaultContext' and its 'Environment' property to prevent potential NullReferenceException when accessing Environment.Name.
AzureSession.Instance.SetProperty(AzureSession.Property.Environment, DefaultProfile.DefaultContext.Environment.Name);
src/Accounts/Authentication/ResourceManager/AzureRmProfile.cs:825
- Verify that concatenating 'organizations' directly to ActiveDirectoryAuthority produces a valid authority URL; consider inserting a '/' if required.
authority = "${sessionEnvironment.ActiveDirectoryAuthority}organizations";
msJinLei
force-pushed
the
clearcontext_fix
branch
from
e786e20 to
fe49e2b
Compare
msJinLei
force-pushed
the
clearcontext_fix
branch
from
fe49e2b to
b517d9f
Compare
msJinLei
force-pushed
the
clearcontext_fix
branch
3 times, most recently
from
4427836 to
82fc7ad
Compare
msJinLei
modified the milestones:
Az 13.5.0 (05/06/2025),
Az 14.0.0 (05/19/2025) ❗Breaking change release❗
msJinLei
force-pushed
the
clearcontext_fix
branch
2 times, most recently
from
d961ba2 to
88f4363
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| var defaultProfile = DefaultProfile as AzureRmProfile; | ||
| if (defaultProfile != null && string.Equals(AzureSession.Instance?.ARMContextSaveMode, "CurrentUser")) | ||
| { | ||
| AzureSession.Instance.SetProperty(AzureSession.Property.Environment, DefaultContext.Environment.Name); |
There was a problem hiding this comment.
Why need to update session in a Get cmdlet which usually doesn't have side effects?
There was a problem hiding this comment.
Why need to update session in a Get cmdlet which usually doesn't have side effects?
The value is used in
which is used inGet-AzContext -ListAvailable -RefreshContextFromTokenCache
There was a problem hiding this comment.
I just feel it strange to set the environment when getting the context. Shouldn't it be set when during connect-azaccount?
There was a problem hiding this comment.
I just feel it strange to set the environment when getting the context. Shouldn't it be set when during connect-azaccount?
It is already set in Connect-AzAccount. But when the customers restart the powershell and run Get-AzContext -ListAvailable -RefreshContextFromTokenCache without login, the contexts are going to be cleared. That's why we have to set it here.
2 participants
Description
The test cases to cover
I
[wam enabled]
II
[wam enabled]
III
[wam disabled]
IV
[wam disabled]
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.mdand reviewed the following information:ChangeLog.mdfile(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.## Upcoming Releaseheader in the past tense.ChangeLog.mdif no new release is required, such as fixing test case only.